-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for the TCP_USER_TIMEOUT option #103
Conversation
Codecov Report
@@ Coverage Diff @@
## master #103 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 98 116 +18
=========================================
+ Hits 98 116 +18
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work @mildsunrise with preparing PR 👏 .
So I suggest we add support for setting the TCP User Timeout on a socket, too.
I have not used TCP_USER_TIMEOUT
before and after reading a bit about it seems like it would complement the rest of the module functinality very well 👍.
Currently only implemented for Linux, I haven't had time to look this on other platforms yet.
So far what I found is that the rfc5482 might be considered "young(ish)" and seems to be either linux specific or just not yet implemented in other kernels.
It's unsupported on freebsd
and darwin
(don't have any strong facts for macos, only few words here and there) but apparently supported on win32
.
- Therefore I suggested throwing in
set
andget
methods if they are called on unsupported platforms. I'm open to suggestions here.
It think it would be great to have an end to end test confirming that it does what it's supposed to do. A test similar to test-tcpdump.js
but only checking that TCP_USER_TIMEOUT
does what it's expected to do on linux
.
A great drill-down by Couldflare regarding this (and many other cases) demonstrates that it could be visualised using tcpdump
.
- Can we create an e2e test using tcpdump to test that it was actually set and at least picked up by kernel?
All in all great job and thanks for the PR I'm all in favor of merging this once we resolve the comments 👌
* Sets the TCP_USER_TIMEOUT value for specified socket. | ||
* | ||
* Note: The msec will be rounded towards the closest integer | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest lightly mentioning the interaction between TCP_USER_TIMEOUT
and TCP_KEEPALIVE
. And provide a link to man page for it. For developer experience purposes.
* | |
* Note: When used with the TCP keepalive option, TCP_USER_TIMEOUT will override | |
keepalive to determine when to close a connection due to keepalive failure. |
@@ -23,6 +24,7 @@ switch (OS.platform()) { | |||
default: | |||
Constants.TCP_KEEPINTVL = 5 | |||
Constants.TCP_KEEPCNT = 6 | |||
Constants.TCP_USER_TIMEOUT = 18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this feature is actually not supported on freebsd
and darwin
(not tested, assumption based, see review comment for details).
For those platforms the value would be undefiend
which might cause segfault and I'm not sure what would happen in that case, most likely not what the user intended to do so I suggest throwing in set
and get
methods if this constant is null
or undefiend
(==null
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot of sense, yes. I'll do that when I find some time.
For now you can ignore the failing check for node |
Co-authored-by: George Hertz <[email protected]>
Co-authored-by: George Hertz <[email protected]>
Thanks for reviewing :)
Okay, I'll try to implement an e2e test using tcpdump, I think it should be doable.
Not exactly... TCP itself defines the user timeout. What RFC5482 does is add an option to the protocol so that you can advertise the timeout to the other peer, rather than just setting it locally. It's not clear how (or if) the
Absolutely! |
That is amazing! I believe we could use some pieces from repo for cloud-flare blog post:
Ah! I see... Thanks for the explanation. 🙏
I'm OK starting with Later we can see if there is a demand for it on other platforms. |
@mildsunrise Gentle ping, should I take over or do you have some progress? |
you can take over :) |
@all-contributors please add @mildsunrise for code, docs, tests |
I've put up a pull request to add @mildsunrise! 🎉 |
keep-alive is usually wanted for dead peer detection. But keep-alive is only one half of that; if the peer dies before acknowledging data, the
TCP_USER_TIMEOUT
option is what decides when the connection gets closed, and that's usually set to 10 minutes. (correct me if wrong)So I suggest we add support for setting the TCP User Timeout on a socket, too. Currently only implemented for Linux, I haven't had time to look this on other platforms yet.